Conversation
There was a problem hiding this comment.
Pull request overview
Updates the UCAN and selector implementations to better match the spec, including stricter command handling, improved selector capabilities, and several correctness/interop fixes.
Changes:
- Tighten command handling by requiring pre-validated
Commandvalues and addingcommand_from_strhelpers. - Extend selectors with byte indexing and Python-style slicing (
[start:end]) plus associated tests/serialization. - Improve spec alignment and cleanup: fix subject-proof check logic, update URLs/typos, adjust wasm timestamp interop, and remove unused modules/files.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| varsig/src/header/traits.rs | Removed obsolete file. |
| varsig/src/hash.rs | Fix spelling in module docs. |
| ucan/src/time/timestamp.rs | Adjust wasm JS Date interop API and conversions. |
| ucan/src/task.rs | Removed module/file. |
| ucan/src/receipt.rs | Removed module/file. |
| ucan/src/lib.rs | Stop exporting removed modules. |
| ucan/src/ipld.rs | Fix spelling in module docs. |
| ucan/src/invocation/builder.rs | Require validated Command; add command_from_str. |
| ucan/src/invocation.rs | Fix subject-proof check logic; fix error formatting; update tests for new command API. |
| ucan/src/delegation/policy/selector/select.rs | Implement slice filter execution and byte indexing; expand tests; adjust proptest cases. |
| ucan/src/delegation/policy/selector/filter.rs | Add Slice filter variant with parsing/display/serde/arbitrary support. |
| ucan/src/delegation/policy/predicate.rs | Spec tweaks: serialization of not == as !=, selector error type fix, spelling fixes. |
| ucan/src/delegation/builder.rs | Require validated Command; add command_from_str. |
| ucan/src/delegation.rs | Update spec link; require exp field on decode; update tests for new command API. |
| ucan/src/crypto/signed.rs | Remove unused signed wrapper type. |
| ucan/src/crypto.rs | Stop exporting removed signed module. |
| ucan/Cargo.toml | Add optional wasm deps and a wasm feature. |
Comments suppressed due to low confidence (1)
ucan/src/invocation.rs:324
- The
CommandMismatcherror message now correctly prints{found:?}, but the construction site setsexpectedtoself.commandandfoundtoproof.command(). Given the check!self.command.starts_with(proof.command()), the expected value should be the proof command (the required prefix) and the found value should be the invocation command. Swapping these will make the error accurate and less confusing to diagnose.
/// Error indicating that the command in the invocation does not match the command in the proof
#[error("command mismatch: expected {expected:?}, found {found:?}")]
CommandMismatch {
/// The expected command
expected: Command,
/// The found command
found: Command,
},
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let millis = date_time.get_time(); | ||
| if millis < 0.0 { | ||
| return Err(OutOfRangeError::BeforeEpoch); | ||
| } | ||
| #[allow(clippy::cast_sign_loss, clippy::cast_possible_truncation)] | ||
| let secs = (millis / 1000.0) as u64; | ||
| Timestamp::from_unix(secs) |
There was a problem hiding this comment.
Timestamp::from_date casts the f64 result of Date::get_time() to u64 without validating it. In JS, invalid dates yield NaN (and could also be ±Infinity), and casting NaN to u64 will currently produce 0, silently turning an invalid date into the Unix epoch. Add an is_finite() / is_nan() guard and return an appropriate error before the cast/conversion.
| // 2⁵³ / 1000 = max seconds that fit in a JS Date without precision loss | ||
| const MAX_SAFE_SECS: u64 = 0x001F_FFFF_FFFF_FFFF / 1000; | ||
| if self.0 > MAX_SAFE_SECS { | ||
| return Err(OutOfRangeError::TooLarge(self.0)); | ||
| } |
There was a problem hiding this comment.
to_date returns OutOfRangeError::TooLarge(self.0) when the timestamp would overflow the milliseconds safe-integer bound for js_sys::Date, but OutOfRangeError::TooLarge is documented as exceeding the 2^53 seconds range. This makes the error semantics/message misleading for callers. Consider adding a dedicated error variant/message for the JS-Date-milliseconds bound (or adjust OutOfRangeError to distinguish seconds vs milliseconds) so the reported reason matches the check being performed.
No description provided.